[MBL-19677][S/P/T] Structured Concurrency foundations #3852

petkybenedek · 2026-01-21T14:46:41Z

What's new

This PR introduces AsyncStore, which makes use of structured concurrency to interact with the API and Core Data.

refs: MBL-19677
builds: Student, Teacher, Parent
affects: Student, Teacher, Parent
release note: none

Checklist

refs: MBL-19677 builds: Student, Teacher, Parent affects: Student, Teacher, Parent release note: none

claude

Review Summary

This PR introduces a new AsyncStore implementation that provides async/await alternatives to the existing reactive Store pattern. The changes align well with Swift's modern concurrency model and follow the project's MVVM architecture.

Issues Found

Critical Bug in NSPersistentContainerExtensions.swift:99-103 - The performWriteTask methods are using self.context instead of the computed context property, which defeats the purpose of having a dedicated write context. This could lead to threading issues.
Memory Leak Risk in AsyncStore.swift:139 - The streamEntitiesFromDatabase() method returns a non-terminating stream without built-in cancellation handling. While the warning comment is present, consider adding safeguards or helper methods.
Silent Error Handling in AsyncStore.swift:147 - The forceRefresh method silently ignores errors with try?. This should at least log errors for debugging purposes.
SwiftLint Violation in AsyncStore.swift:54 - Line length exceeds the project's limit and should be broken into multiple lines.

Positive Aspects

Clean async/await API: The new AsyncStore provides a modern, ergonomic interface that will improve code readability compared to Combine publishers.
Offline mode support: Proper integration with OfflineModeInteractor ensures consistent behavior across online/offline states.
Smart caching: The cache validation logic is well-implemented with hasCacheExpired checks before making API calls.
Consistent error handling: Most methods properly propagate errors through async throws, giving callers control over error handling.
Pagination support: The loadAllPages parameter provides flexibility for different data loading scenarios.
Good separation of concerns: Private helper methods like fetchEntitiesFromCache, fetchEntitiesFromAPI, and fetchEntitiesFromDatabase keep the code organized and testable.

Recommendations

Fix the critical bug: The performWriteTask implementation must use the write context, not the view context.
Add unit tests: Given the complexity of async streams and Core Data operations, comprehensive unit tests are essential. Follow the conventions in CLAUDE-unit-tests.md.
Consider adding cancellation helpers: For non-terminating streams, provide convenience methods that handle cancellation automatically (e.g., with timeouts or lifecycle-bound cancellation).
Add logging: At minimum, log errors in forceRefresh to aid debugging.
SwiftLint compliance: Ensure all code passes SwiftLint checks before merging.

claude · 2026-01-21T14:48:14Z

Core/Core/Common/CommonModels/Store/AsyncStore.swift

+    public func getEntities(ignoreCache: Bool = false, loadAllPages: Bool = true) async throws -> [U.Model] {
+        let scope = useCase.scope
+        let request = NSFetchRequest<U.Model>(entityName: String(describing: U.Model.self))
+        request.predicate = scope.predicate


SwiftLint violation: This line exceeds the line length limit. Consider breaking it into multiple lines for better readability.

claude · 2026-01-21T14:48:15Z

Core/Core/Common/CommonModels/Store/AsyncStore.swift

+
+    /// - Warning: This stream **does not terminate**. Ensure proper cancellation of its consuming task.
+    public func streamEntitiesFromDatabase() throws -> AsyncThrowingStream<[U.Model], Error> {
+        let scope = useCase.scope


Critical: This stream does not terminate and can cause memory leaks if not properly cancelled. Consider:

Adding a timeout mechanism

Providing a helper method that handles cancellation

Adding unit tests that verify proper cleanup when the consuming task is cancelled

claude · 2026-01-21T14:48:16Z

Core/Core/Common/CommonModels/Store/AsyncStore.swift

+        return Self.streamEntitiesFromDatabase(fetchRequest: request, context: context)
+    }
+
+    /// Refreshes the entities by requesting the latest data from the API.


The error from forceRefresh is being silently ignored with try?. This could hide important issues. Consider:

Logging the error for debugging

Using a completion handler or throwing the error to the caller

At minimum, adding a comment explaining why it's safe to ignore errors here

claude · 2026-01-21T14:48:17Z

Core/Core/Common/Extensions/CoreData/NSPersistentContainerExtensions.swift


    @objc open func performWriteTask(_ block: @escaping (NSManagedObjectContext) -> Void) {
-        let context = writeContext ?? {
+        context.perform { block(self.context) }


Bug: This implementation is incorrect. The method is supposed to use a write context, but it's always using self.context instead of the actual context variable returned by the context computed property.

The block(self.context) should be block(context) where context is assigned from the context computed property.

claude · 2026-01-21T14:48:19Z

Core/Core/Common/Extensions/CoreData/NSPersistentContainerExtensions.swift

+    }
+
+    public func performWriteTask<T>(_ block: @escaping (NSManagedObjectContext) throws -> T) async rethrows -> T {
+        return try await context.perform { try block(self.context) }


Bug: Same issue here - this should use the write context from the context computed property, not self.context. The correct implementation should be:

let writeContext = context return try await writeContext.perform { try block(writeContext) }

inst-danger · 2026-01-21T14:51:31Z

	Warnings
⚠️	This pull request will not generate a release note.

Affected Apps: Student, Teacher, Parent

Builds: Student, Teacher, Parent

MBL-19677

Coverage	New %	Master %	Delta
Canvas iOS	91.38%	81.06%	10.31%

Generated by 🚫 dangerJS against 6d1c18a

inst-danger · 2026-01-21T15:21:10Z

Builds

Commit: Add equatable conformance (6d1c18a)
Build Number: 1227
Built At: Jan 29 16:49 CET (01/29 08:49 AM MST)

Student	Teacher	Parent

suhaibabsi-inst · 2026-01-29T16:17:42Z

Core/Core/Common/CommonModels/Store/AsyncStore.swift

+        let entities = try await getEntities(ignoreCache: ignoreCache, loadAllPages: loadAllPages)
+
+        if assertOnlyOneEntityFound, entities.count > 1 { throw AsyncStoreError.moreThanOneEntityFound(entities.count) }
+        guard let entity = entities.first else { throw AsyncStoreError.noEntityFound }


So, here we are treating the empty data (no entities) as failure. Wouldn't that confuse code at call sites? As that would often be treated as a separate case in terms of UI ?

suhaibabsi-inst

@petkybenedek Great Work! I liked how AsyncFetchedResults turned to be. Though I have a couple of comments to consider to fully utilize the syntactic beauty of async/await coding.

suhaibabsi-inst · 2026-01-29T18:31:26Z

Core/Core/Common/CommonModels/Store/AsyncFetchedResults.swift

+import Foundation
+import CoreData
+
+@preconcurrency


I was thinking if we can get rid of this @preconcurrency attribute be used with our newly introduced Async types. How about using it with @preconcurrency import CoreData as that was by Apple, and then move the fetch method to an extension of NSManagedObjectContext.

@preconcurrency import CoreData public final class AsyncFetchResults<ResultType: NSFetchRequestResult> { .... public func fetch() async throws -> [ResultType] { try await context.fetch(request) } } extension NSManagedObjectContext { public func fetch<R: NSFetchRequestResult>(_ request: NSFetchRequest<R>) async throws -> [R] { try await perform { return try self.fetch(request) } } }

This way we keep the new code committed to the async/await model, isolating any source of pre-concurrency as much as possible.

suhaibabsi-inst · 2026-01-29T18:39:52Z

Core/Core/Common/CommonModels/Store/AsyncStore.swift

+    ///       Defaults to **false**.
+    ///     - loadAllPages: Tells the request if it should load all the pages or just the first one. Defaults to **true**.
+    /// - Returns: An async sequence of list of entities.
+    public func streamEntities(ignoreCache: Bool = false, loadAllPages: Bool = true) async throws -> AsyncThrowingStream<[U.Model], Error> {


How about renaming it to be more indicative of watching store updates. Something like:
updatesStream(), or simply ``updates()`.

suhaibabsi-inst · 2026-01-29T18:46:20Z

Core/Core/Common/CommonModels/Store/AsyncStore.swift

+            )
+        } else {
+            if ignoreCache {
+                try await Self.streamEntitiesFromAPI(


How about turning this to simply be:
func updateEntitiesFromAPI() async throws, then after it is finished you simply call Self.streamEntitiesFromDatabase(fetchRequest: fetchRequest, context: context)

try await updateEntitiesFromAPI() try await Self.streamEntitiesFromDatabase(fetchRequest: fetchRequest, context: context)

with async/await we get to isolate async work in a better way than before.

We even can re-write the whole function to git rid of Self.streamEntitiesFromCache:

public func updatesStream(ignoreCache: Bool = false, loadAllPages: Bool = true) async throws -> AsyncThrowingStream<[U.Model], Error> { if offlineModeInteractor?.isOfflineModeEnabled() == true { return Self.streamEntitiesFromDatabase( fetchRequest: request, context: context ) } else { let hasExpired = await useCase.hasCacheExpired(environment: environment) if ignoreCache || hasExpired { try await Self.updateEntitiesFromAPI( useCase: useCase, loadAllPages: loadAllPages, fetchRequest: request, context: context, environment: environment ) } return Self.streamEntitiesFromDatabase( fetchRequest: request, context: context ) } }

suhaibabsi-inst · 2026-01-29T18:59:31Z

Core/Core/Common/CommonModels/Store/AsyncStore.swift

+        let scope = useCase.scope
+        let request = NSFetchRequest<U.Model>(entityName: String(describing: U.Model.self))
+        request.predicate = scope.predicate
+        request.sortDescriptors = scope.order


This can be refactored out to a computed property or set at the initializer:

private var request: NSFetchRequest<U.Model> { let scope = useCase.scope let request = NSFetchRequest<U.Model>(entityName: String(describing: U.Model.self)) request.predicate = scope.predicate request.sortDescriptors = scope.order return request }

suhaibabsi-inst · 2026-01-29T19:02:44Z

Core/Core/Common/CommonModels/Store/AsyncStore.swift

+    ) async throws -> [T] {
+        let hasExpired = await useCase.hasCacheExpired(environment: environment)
+
+        return if hasExpired {


We should avoid using if for returning or setting a value. If necessary use it in a non-escaping closure:

let someValue = { if condition { val1 } else { val2 } }()

Create AsyncStore

f0c61ea

refs: MBL-19677 builds: Student, Teacher, Parent affects: Student, Teacher, Parent release note: none

claude bot reviewed Jan 21, 2026

View reviewed changes

petkybenedek changed the title ~~[MBL-19677] [S/P/T] Structured Concurrency foundations~~ [MBL-19677][S/P/T] Structured Concurrency foundations Jan 21, 2026

Correct renaming

a179ad8

petkybenedek self-assigned this Jan 22, 2026

petkybenedek added 4 commits January 26, 2026 14:22

Created unit tests for AsyncStore

1595afa

Remove newline

666c444

Create AsyncFecthedResults and add getFirstEntity

4abcbb4

Add equatable conformance

6d1c18a

petkybenedek marked this pull request as ready for review January 29, 2026 15:58

petkybenedek requested review from Ahmed-Naguib93, JacksonHowe, ktheory, rh12, suhaibabsi-inst, szabinst and vargaat as code owners January 29, 2026 15:58

petkybenedek removed request for Ahmed-Naguib93, JacksonHowe, ktheory and szabinst January 29, 2026 16:14

suhaibabsi-inst reviewed Jan 29, 2026

View reviewed changes

[MBL-19677][S/P/T] Structured Concurrency foundations #3852

Are you sure you want to change the base?

[MBL-19677][S/P/T] Structured Concurrency foundations #3852

Uh oh!

Conversation

petkybenedek commented Jan 21, 2026

What's new

Checklist

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Review Summary

Issues Found

Positive Aspects

Recommendations

Uh oh!

claude bot Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

inst-danger commented Jan 21, 2026 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Affected Apps: Student, Teacher, Parent

Builds: Student, Teacher, Parent

Uh oh!

inst-danger commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Builds

Uh oh!

suhaibabsi-inst Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

suhaibabsi-inst left a comment

Choose a reason for hiding this comment

Uh oh!

suhaibabsi-inst Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

suhaibabsi-inst Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

suhaibabsi-inst Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

suhaibabsi-inst Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suhaibabsi-inst Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

suhaibabsi-inst Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

inst-danger commented Jan 21, 2026 •

edited by atlassian bot

Loading

inst-danger commented Jan 21, 2026 •

edited

Loading

suhaibabsi-inst Jan 29, 2026 •

edited

Loading